Add resident device change call #1517

lslusarczyk · 2025-09-11T12:30:55Z

UMF part of https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/supported/sycl_ext_oneapi_peer_access.asciidoc feature.

UR/llvm part is in intel/llvm#19257

include/umf/memory_pool_ops.h

src/libumf.map

src/memory_provider.c

src/provider/provider_level_zero.c

src/provider/provider_cuda.c

src/provider/provider_tracking.c

test/common/provider_null.c

src/provider/provider_level_zero.c

src/critnib/critnib.c

src/libumf.map

src/libumf.def

src/memory_provider.c

include/umf/memory_provider_ops.h

include/umf/providers/provider_level_zero.h

ldorau

14 / 16 files reviewed (will be continued)

include/umf/memory_provider_ops.h

lplewa · 2025-09-19T13:16:18Z

I still oppose against adding provider specific code to ops interface.
If you don't want to do it thru CTL just implement it as umfLevelZeroMemoryProviderResidentDeviceChange() in provider_level_zero.h which do not go throught OPS. This function will act as an "level zero specific api extension".

note: remember that you will have to call umfMemoryProviderGetPriv() in this function to retrieve ze_memory_provider_t from provider handle

src/libumf.def

src/provider/provider_level_zero.c

lplewa · 2025-09-19T13:21:25Z

src/provider/provider_level_zero.c


 static void init_ze_global_state(void) {
+
+    char *lib_name = getenv("UMF_ZE_LOADER_LIB_NAME");


Can you do this thru CTL please.

We created CTL to limit new envvariables.

This is for tests only. Env variables are simple to write and to understand for future maintainers. Could you please accept it as is in this PR? But I am OK if anyone wants to change it to CTL after this PR is merged.

src/provider/provider_level_zero.c

lplewa · 2025-09-19T13:33:09Z

src/provider/provider_level_zero.c

+
+    // TODO: add assertions to UMF and change it to be an assertion
+    if (info->props.base != (void *)key) {
+        LOG_ERR("key:%p is different than base:%p", (void *)key,


ok, applied

lplewa · 2025-09-19T13:37:40Z

src/utils/utils_log.h

-#define LOG_DEBUG(...) utils_log(LOG_DEBUG, __func__, __VA_ARGS__);
-#define LOG_INFO(...) utils_log(LOG_INFO, __func__, __VA_ARGS__);
-#define LOG_WARN(...) utils_log(LOG_WARNING, __func__, __VA_ARGS__);
-#define LOG_ERR(...) utils_log(LOG_ERROR, __func__, __VA_ARGS__);
-#define LOG_FATAL(...) utils_log(LOG_FATAL, __func__, __VA_ARGS__);
-
-#define LOG_PDEBUG(...) utils_plog(LOG_DEBUG, __func__, __VA_ARGS__);
-#define LOG_PINFO(...) utils_plog(LOG_INFO, __func__, __VA_ARGS__);
-#define LOG_PWARN(...) utils_plog(LOG_WARNING, __func__, __VA_ARGS__);
-#define LOG_PERR(...) utils_plog(LOG_ERROR, __func__, __VA_ARGS__);
-#define LOG_PFATAL(...) utils_plog(LOG_FATAL, __func__, __VA_ARGS__);
+#ifdef UMF_DEVELOPER_MODE
+#define UMF_STRINGIFY(x) #x
+#define UMF_TOSTRING(x) UMF_STRINGIFY(x)
+#define UMF_FUNC_DESC() __FILE__ ":" UMF_TOSTRING(__LINE__)
+#else
+#define UMF_FUNC_DESC() __func__
+#endif
+
+#define LOG_DEBUG(...) utils_log(LOG_DEBUG, UMF_FUNC_DESC(), __VA_ARGS__);
+#define LOG_INFO(...) utils_log(LOG_INFO, UMF_FUNC_DESC(), __VA_ARGS__);
+#define LOG_WARN(...) utils_log(LOG_WARNING, UMF_FUNC_DESC(), __VA_ARGS__);
+#define LOG_ERR(...) utils_log(LOG_ERROR, UMF_FUNC_DESC(), __VA_ARGS__);
+#define LOG_FATAL(...) utils_log(LOG_FATAL, UMF_FUNC_DESC(), __VA_ARGS__);
+
+#define LOG_PDEBUG(...) utils_plog(LOG_DEBUG, UMF_FUNC_DESC(), __VA_ARGS__);
+#define LOG_PINFO(...) utils_plog(LOG_INFO, UMF_FUNC_DESC(), __VA_ARGS__);
+#define LOG_PWARN(...) utils_plog(LOG_WARNING, UMF_FUNC_DESC(), __VA_ARGS__);
+#define LOG_PERR(...) utils_plog(LOG_ERROR, UMF_FUNC_DESC(), __VA_ARGS__);
+#define LOG_PFATAL(...) utils_plog(LOG_FATAL, UMF_FUNC_DESC(), __VA_ARGS__);


This should be a separate PR.

There are comments which i whould make - but i strongly believe that this discussion should happen in the separate PR to do not block this PR

lplewa · 2025-09-19T14:05:12Z

src/provider/provider_level_zero.c

+        // found
+        if (is_adding) {
+            utils_write_unlock(&ze_provider->resident_device_rwlock);
+            // impossible for UR, should be an assertion


imho we should not assume that user will not do something then is should be an assertion. If it comes it for any kind of user it should be validated an correct error should be returned

ok, comment removed

lplewa · 2025-09-19T14:09:47Z

src/provider/provider_level_zero.c

+
+    uint32_t existing_peer_index = 0;
+    utils_write_lock(&ze_provider->resident_device_rwlock);
+    while (existing_peer_index < ze_provider->resident_device_count &&


NIT: this code whould be much easier to read if it whould be a for loop

for (index = 0; index < count; index++) if (device[index] == index) break;

I need for loop index to be available outside the loop. I don't think it is more readable. Left as is.

you do the incrementation within the while loop and have extra && to mark the stop, I believe I agree with Łukasz - this would look better as for.

// you obviously don't have to init anything in for (e.g. for(; index < count; index++)

Yes i noticed that you need it outside of loop - this is why i write
for(index = 0; index < count; index++) if (device[index] == index) break;
not
for (int index = 0; index < count; index++) if (device[index] == index) break;

If something is for loop we should use for not while. If you need access to loop counter outside of function you can declare it before loop - this is a standard approach which should not confuse reader.

lplewa · 2025-09-19T14:36:06Z

src/provider/provider_level_zero.c

+            const uint32_t new_capacity =
+                ze_provider->resident_device_capacity * 2 +
+                1; // +1 to work also with old capacity == 0
+            ze_device_handle_t *new_handles =


Do this code is performance critical, or will be called multiple times

If not i would prefer to have this array just always reallocated by one,
If yes can we abstract this vector like structure to separate module, or even better just use list structure with is included in umf.

ok, not critical, applied

lplewa · 2025-09-19T14:39:34Z

src/provider/provider_level_zero.c

+        .failed_changes = 0,
+    };
+
+    umf_result_t result = umfMemoryTrackerIterateAll(


This is "hacky" and it will not work if someone uses pool without tracker or just use provider without pool.

We allready solved this issue in other providers were we keep track allocation by the provider (for example please check os_provider)

I was told by you, that I can use tracker. Since it is not a case now that l0 provider is used without tracker, so I am only leaving a comment here that warns about what you noticed.

I think we miss communicated - I will follow up on this later on.

lslusarczyk · 2025-09-22T13:45:24Z

I still oppose against adding provider specific code to ops interface. If you don't want to do it thru CTL just implement it as umfLevelZeroMemoryProviderResidentDeviceChange() in provider_level_zero.h which do not go throught OPS. This function will act as an "level zero specific api extension".

note: remember that you will have to call umfMemoryProviderGetPriv() in this function to retrieve ze_memory_provider_t from provider handle

Great! Using umfLevelZeroMemoryProviderResidentDeviceChange simplified code even more than using ops. Applied with pleasure.

test/common/ze_loopback.cpp

test/common/level_zero_mocks.h

test/common/ze_loopback.h

test/common/level_zero_mocks.cpp

test/pools/pool_residency.cpp

src/provider/provider_level_zero.c

lslusarczyk · 2025-10-01T08:37:14Z

@lukaszstolarczuk , @bratpiorka , @ldorau ,
CI passed, please check if I applied your comments correctly, resolve discussions which can be resolved and give approve if there is nothing left to do with respect to your comments

@lplewa , please do the same wrt your comments when you're back

test/test_valgrind.sh

test/common/level_zero_mocks.cpp

src/libumf.def

include/umf/memory_provider_ops.h

src/provider/provider_level_zero.c

lukaszstolarczuk · 2025-10-01T14:48:41Z

src/provider/provider_level_zero.c

+
+    uint32_t existing_peer_index = 0;
+    utils_write_lock(&ze_provider->resident_device_rwlock);
+    while (existing_peer_index < ze_provider->resident_device_count &&


you do the incrementation within the while loop and have extra && to mark the stop, I believe I agree with Łukasz - this would look better as for.

// you obviously don't have to init anything in for (e.g. for(; index < count; index++)

src/provider/provider_level_zero.c

test/common/CMakeLists.txt

test/common/level_zero_mocks.cpp

test/common/level_zero_mocks.h

test/common/ze_loopback.cpp

test/pools/pool_residency.cpp

lplewa

Please refactor commits structure.

commits in PR should be split to functional parts, with proper commit message.
Each commit should work (at least it should build - but we do not check it so exception happens). You should not split implementation, tests, documentation to separate commits.

If i were you i would create(This list is an inspiration for proper commits structure, you do not have split your PR to exactly this commits) : commit for adding iterate over critnib, commit for changes in asserts, maybe commit for this mock library, commit for allowing ovveride lib_name, commit for change in setResidentDevides, commit for main functionality of this PR....

include/umf/memory_pool_ops.h

src/critnib/critnib.c

lplewa · 2025-10-06T12:46:26Z

src/provider/provider_level_zero.c

+
+    uint32_t existing_peer_index = 0;
+    utils_write_lock(&ze_provider->resident_device_rwlock);
+    while (existing_peer_index < ze_provider->resident_device_count &&


Yes i noticed that you need it outside of loop - this is why i write
for(index = 0; index < count; index++) if (device[index] == index) break;
not
for (int index = 0; index < count; index++) if (device[index] == index) break;

If something is for loop we should use for not while. If you need access to loop counter outside of function you can declare it before loop - this is a standard approach which should not confuse reader.

lplewa · 2025-10-06T12:57:25Z

src/provider/provider_level_zero.c

+        LOG_ERR("umfMemoryTrackerIterateAll did not manage to do some change "
+                "numFailed: %d, numSuccess: %d",
+                privData.success_changes, privData.failed_changes);
+        return UMF_RESULT_ERROR_MEMORY_PROVIDER_SPECIFIC;


When returining this error code get_last_native_error function should be able to return more precise error message - please add support for it.

ok, added store_last_native_error(result); in ze_memory_provider_resident_device_change_helper

lplewa · 2025-10-06T12:58:37Z

src/provider/provider_level_zero.c

+        .failed_changes = 0,
+    };
+
+    umf_result_t result = umfMemoryTrackerIterateAll(


I think we miss communicated - I will follow up on this later on.

lplewa · 2025-10-06T13:02:03Z

src/provider/provider_level_zero.c

+    return 0;
+}
+
+umf_result_t umfLevelZeroMemoryProviderResidentDeviceChange(


I would prefer to have add and remove function - i recall pbalcer already suggested it too.

I will split it to 3 functions - add, delete, and helper function to iterate over existing allocations (so we limit code duplication)

lslusarczyk · 2025-10-06T14:32:29Z

Please refactor commits structure.

commits in PR should be split to functional parts, with proper commit message. Each commit should work (at least it should build - but we do not check it so exception happens). You should not split implementation, tests, documentation to separate commits.

If i were you i would create(This list is an inspiration for proper commits structure, you do not have split your PR to exactly this commits) : commit for adding iterate over critnib, commit for changes in asserts, maybe commit for this mock library, commit for allowing ovveride lib_name, commit for change in setResidentDevides, commit for main functionality of this PR....

Refactor will be squashing all commits to one during merge.
Each commit will be working :)

lplewa · 2025-10-06T14:34:02Z

Please refactor commits structure.
commits in PR should be split to functional parts, with proper commit message. Each commit should work (at least it should build - but we do not check it so exception happens). You should not split implementation, tests, documentation to separate commits.
If i were you i would create(This list is an inspiration for proper commits structure, you do not have split your PR to exactly this commits) : commit for adding iterate over critnib, commit for changes in asserts, maybe commit for this mock library, commit for allowing ovveride lib_name, commit for change in setResidentDevides, commit for main functionality of this PR....

Refactor will be squashing all commits to one during merge. Each commit will be working :)

We do not do squash merge in this project

lslusarczyk requested a review from a team as a code owner September 11, 2025 12:30

bratpiorka requested changes Sep 11, 2025

View reviewed changes

bratpiorka requested review from lplewa and ldorau September 11, 2025 12:53

bratpiorka reviewed Sep 12, 2025

View reviewed changes

src/provider/provider_level_zero.c Outdated Show resolved Hide resolved

bratpiorka reviewed Sep 12, 2025

View reviewed changes

src/provider/provider_level_zero.c Outdated Show resolved Hide resolved

bratpiorka reviewed Sep 12, 2025

View reviewed changes

src/provider/provider_level_zero.c Outdated Show resolved Hide resolved

bratpiorka reviewed Sep 12, 2025

View reviewed changes

src/provider/provider_level_zero.c Show resolved Hide resolved

bratpiorka reviewed Sep 12, 2025

View reviewed changes

src/provider/provider_level_zero.c Outdated Show resolved Hide resolved

bratpiorka reviewed Sep 12, 2025

View reviewed changes

src/provider/provider_level_zero.c Outdated Show resolved Hide resolved

bratpiorka reviewed Sep 12, 2025

View reviewed changes

src/provider/provider_level_zero.c Outdated Show resolved Hide resolved

bratpiorka reviewed Sep 12, 2025

View reviewed changes

src/provider/provider_level_zero.c Outdated Show resolved Hide resolved

bratpiorka reviewed Sep 12, 2025

View reviewed changes

src/provider/provider_level_zero.c Outdated Show resolved Hide resolved

bratpiorka reviewed Sep 12, 2025

View reviewed changes

src/provider/provider_level_zero.c Outdated Show resolved Hide resolved

lslusarczyk force-pushed the memresident branch from d53a23f to 9d7e12c Compare September 15, 2025 08:16

lukaszstolarczuk reviewed Sep 15, 2025

View reviewed changes

src/critnib/critnib.c Outdated Show resolved Hide resolved

src/libumf.map Outdated Show resolved Hide resolved

src/libumf.def Outdated Show resolved Hide resolved

src/memory_provider.c Outdated Show resolved Hide resolved

lplewa requested changes Sep 15, 2025

View reviewed changes

include/umf/memory_provider_ops.h Outdated Show resolved Hide resolved

include/umf/providers/provider_level_zero.h Outdated Show resolved Hide resolved

lslusarczyk mentioned this pull request Sep 15, 2025

memory resident limit to enabled peers intel/llvm#19257

Draft

ldorau reviewed Sep 15, 2025

View reviewed changes

pbalcer reviewed Sep 16, 2025

View reviewed changes

include/umf/memory_provider_ops.h Outdated Show resolved Hide resolved

lslusarczyk force-pushed the memresident branch from 88f672b to 53ec753 Compare September 19, 2025 12:00

bratpiorka reviewed Sep 19, 2025

View reviewed changes

src/libumf.def Outdated Show resolved Hide resolved

bratpiorka reviewed Sep 19, 2025

View reviewed changes

src/provider/provider_level_zero.c Outdated Show resolved Hide resolved

lplewa requested changes Sep 19, 2025

View reviewed changes

lukaszstolarczuk reviewed Sep 22, 2025

View reviewed changes

lukaszstolarczuk reviewed Oct 1, 2025

View reviewed changes

bratpiorka requested changes Oct 2, 2025

View reviewed changes

lplewa requested changes Oct 6, 2025

View reviewed changes

bratpiorka approved these changes Oct 7, 2025

View reviewed changes

lslusarczyk force-pushed the memresident branch from ee516a4 to dbe3b88 Compare October 7, 2025 16:17

Add resident device change call

96e69c4

lslusarczyk force-pushed the memresident branch from e2b49a4 to 96e69c4 Compare October 7, 2025 16:28


		static void init_ze_global_state(void) {

		char *lib_name = getenv("UMF_ZE_LOADER_LIB_NAME");

Add resident device change call #1517

Are you sure you want to change the base?

Add resident device change call #1517

Uh oh!

Conversation

lslusarczyk commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldorau left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lplewa commented Sep 19, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lslusarczyk Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lslusarczyk commented Sep 11, 2025 •

edited

Loading

ldorau left a comment •

edited

Loading

lslusarczyk Sep 22, 2025 •

edited

Loading